Conversation
…ough the redaction of a connect failure reported() rebuilds a connect failure link by link wherever it may carry the credentials of the backend - and on every chain longer than the walk looking for them, which answers "yes" past its 32 links whether the url has a password or not. The rebuild made every link a plain SQLException and put a bare one with no SQLState where its own budget ran out, so a link saying the connection is gone by its type, or standing past the 32nd, read false through isConnectionFailure(): write() neither replayed the attempt nor distrusted the pool over a catalog connect refused the way a shorter chain is replayed. Every rebuilt link now keeps the standard JDBC type it extends, and the link standing for what the budget cut carries the type, SQLState and vendor code of the first link of the rest that says the connection is gone - and says nothing of it where the rest does not, so the cut makes no failure a drop either. Fixes OpenIdentityPlatform#1074
maximthomas
left a comment
There was a problem hiding this comment.
praise: The rebuild now carries what says the connection is gone, and it asks the classifier's own question to decide that.
droppedTail()asksJDBCStorage.saysTheConnectionIsGone(), now package-private, rather than a second copy of the rule.testALongChainThatSaysNothingOfTheConnectionIsNotMadeToSayItByTheRebuildguards the other direction too: the cut makes no failure a drop.sameKind()checks each subtype before its supertype: the three connection types come first, andSQLTimeoutException/SQLTransactionRollbackExceptioncome beforeSQLTransientException.
suggestion (non-blocking): Only the next-exception cut is pinned. The cause cuts, the suppressed cut and the walk's cause/suppressed enqueues are not.
opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:2239, :2301, :2364, :2344-2346
Every new fixture (plainChain, deepChain) is chained with setNextException only, so the gone link is always reached through the :2235 arm. redactedCopy() rebuilds the next chain first, then the cause, then the suppressed. On a next chain of 32+ links, the budget is therefore spent before the head's own cause and suppressed are reached. The suppressed case is where establish() puts the 08003 close failure (#929). If droppedTail(Collections.<Throwable>emptyList()) is put at :2239, :2301 or :2364, or the cause/suppressed enqueue is dropped from firstLinkSayingTheConnectionIsGone(), #1074 comes back on those edges and the suite stays green. The older long cause chains (CachedConnectionTestCase:888/:910/:930) are 08006 at every link and never call isConnectionFailure. I established this by reading, not by running the mutant.
@Test(timeOut = 60000)
public void testALinkThatSaysTheConnectionIsGoneOnACauseOrASuppressedEdgePastTheBudgetStillSaysSo()
throws Exception {
final String url = "jdbc:postgresql://127.0.0.1:5432/opendj";
final SQLException byCause = plainChain(40);
byCause.initCause(new java.io.IOException("socket closed", new SQLException("connection reset", "08S01", 10054)));
final SQLException bySuppressed = plainChain(40);
bySuppressed.addSuppressed(new SQLException("the connection is closed", "08003"));
for (final SQLException failure : new SQLException[] { byCause, bySuppressed }) {
final SQLException reported = CachedConnection.reported(failure, url);
assertNotSame(reported, failure, "a chain this long is expected to be rebuilt");
assertTrue(JDBCStorage.isConnectionFailure(reported),
(failure == byCause ? "the cause" : "the suppressed") + " cut past the budget no longer says the connection is gone");
}
}Pin: byCause turns red under an empty rest at :2239 and under a dropped cause enqueue; bySuppressed turns red under an empty rest at :2301. Both are green at this head.
suggestion (non-blocking): Only the three connection arms of sameKind() are pinned. Any of the other nine can be deleted or reordered with every test still green.
opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:2252-2289
The only typed exceptions that go through reported() in a test are the three at CachedConnectionTestCase:956. The arms are not free of behaviour. Delete the SQLTimeoutException arm and a driver timeout on a url with a password becomes a SQLTransientException. It then takes the warn arm of borrowedOrShared() (JDBCStorage.java:7104) instead of the debug one, which the same failure takes on a url without a password.
@Test(timeOut = 60000)
public void testEveryStandardTypeOfALinkIsKeptThroughRedaction() throws Exception {
final String url = "jdbc:postgresql://opendj:S3cretOfTheBackend@127.0.0.1:5432/opendj";
final String message = "login to " + url + " failed";
for (final SQLException original : new SQLException[] {
new SQLTimeoutException(message, "HYT00", 0),
new java.sql.SQLTransactionRollbackException(message, "40001", 1205),
new java.sql.SQLFeatureNotSupportedException(message, "0A000", 0),
new java.sql.SQLIntegrityConstraintViolationException(message, "23000", 2627),
new java.sql.SQLInvalidAuthorizationSpecException(message, "28000", 18456),
new java.sql.SQLSyntaxErrorException(message, "42000", 0),
new java.sql.SQLDataException(message, "22000", 0),
new java.sql.SQLTransientException(message, "S1000", 0),
new java.sql.SQLNonTransientException(message, "S1000", 0) }) {
final SQLException reported = CachedConnection.reported(original, url);
assertNoCredentials(reported);
assertEquals(reported.getClass(), original.getClass(), "the rebuild changed the type of the link");
}
}Pin: deleting any one of the nine arms turns this case red.
suggestion (non-blocking): The direct-throw deep-chain case does not check that the refusal took the direct road.
opendj-server-legacy/src/test/java/org/opends/server/backends/jdbc/CatalogConnectionTestCase.java:703
The case catches a plain SQLException, and that also admits the SQLTimeoutException of catalogConnectTimedOut(), whose cause is reported(last) and carries the same tail. Suppose 08S01 or 10054 becomes worth retrying for POSTGRES. The connect then retries to the 60 s deadline, the case still goes green, and nothing exercises the direct throw at JDBCStorage.java:2135 any more. The interrupt case of the same file guards its road at :335.
} catch (SQLException expected) {
assertFalse(expected instanceof SQLTimeoutException,
"the refusal was retried to the deadline instead of being reported at once");suggestion (non-blocking): firstLinkSayingTheConnectionIsGone() walks the chain a second time. The predicate is shared, but the walk is not.
opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/CachedConnection.java:2329, JDBCStorage.java:4844, :4888
The PR makes saysTheConnectionIsGone() package-private so that "a second copy of the question would [not] drift", but it copies the walk around it. isConnectionFailure() goes through firstLinkMatching(…, WITH_THE_RELEASE, EVERY_LINK, …) into walkLinks(), whose javadoc calls it "the single traversal of this class". The two give the same yes/no today. A later change to the edges of walkLinks() would change what write() reads and leave what the tail carries as it was.
// JDBCStorage
static SQLException connectionFailureLink(Throwable failure) {
return firstLinkMatching(failure, WITH_THE_RELEASE, EVERY_LINK, JDBCStorage::saysTheConnectionIsGone);
}
static boolean isConnectionFailure(Throwable failure) {
return connectionFailureLink(failure)!=null;
} // CachedConnection.droppedTail(rest), in place of firstLinkSayingTheConnectionIsGone(rest)
SQLException gone = null;
for (final Throwable t : rest) {
gone = JDBCStorage.connectionFailureLink(t);
if (gone != null) {
break;
}
}issue (non-blocking): The borrowedOrShared() javadoc still says that a failure naming the credentials is rebuilt as a plain SQLException.
opendj-server-legacy/src/main/java/org/opends/server/backends/jdbc/JDBCStorage.java:7077-7079
Since sameKind() (CachedConnection.java:2230), the rebuild keeps the standard JDBC type. A driver SQLTimeoutException on a url with a password now takes the debug arm at :7104, not the warn arm. The paragraph's conclusion still rests on #1011; only this premise is out of date.
* a driver of no known dialect has no vendor code read at all (issue #1011). The type is no
* rule either: a driver need not raise the standard JDBC type that names what happened, and a
* failure whose chain names the credentials of the backend keeps only that standard type, not
* the class of the driver (#1074). Sorting them here would be that
Fixes #1074
The problem
CachedConnection.reported()rebuilds a connect failure link by link, andJDBCStorage.write()reads the rebuilt chain throughisConnectionFailure()whenever the catalog connect fails inside a write (newCatalogConnection(): the direct throw, the interrupted wait, andcatalogConnectTimedOut(), whose cause isreported(last)). Two things in that rebuild answered the question with thefalseof a failure that says nothing about the connection:redactedCopy()runs out of its 32 links it putsdroppedTail()in place of the rest - a bareSQLExceptionwith no SQLState, whichsaysTheConnectionIsGone()can never accept. A class 08 link past the 32nd was lost.new SQLException(msg, state, code), so aSQLRecoverableException/SQLNonTransientConnectionException/SQLTransientConnectionExceptionthat says the connection is gone by its type alone - whichsaysTheConnectionIsGone()asks before the SQLState - lost it. The reviewer of [#961] Walk every link of a failure where the verdict decides something #1004 noted this in the thought that became JDBC backend: reported() drops the SQLState of a connection-is-gone link past MAX_CHAIN_LENGTH, so a redacted catalog-connect failure reads false through isConnectionFailure() #1074; the issue text kept only the tail.Two corrections to the issue text, both pinned by the tests below:
holdsCredentials()answers "yes" past its own 32-link walk, so every chain longer than that is rebuilt whether the url has a password or not. The case the issue needs - a deciding link past 32 others - is therefore rebuilt on every deployment; the "only where the failure names the credentials" narrowing does not hold. (I made the same mistake in my reply on [#961] Walk every link of a failure where the verdict decides something #1004.)What was lost:
replayReason()did not replay the attempt as "a connection the database dropped", and the pool was not told - where the same refusal a few links shorter, or on a url without a password, is.The fix
sameKind(): every rebuilt link keeps the standard JDBC type it extends (the three connection types,SQLTimeoutException,SQLTransactionRollbackException,SQLFeatureNotSupportedException, the other standardSQLNonTransient*/SQLTransient*subtypes, elseSQLException). Not the driver's own class, which this cannot be sure of building.droppedTail(rest): walks the links it stands for to their end - by the edgesisConnectionFailure()walks (cause, next exception, suppressed), identity-visited, building nothing - and where one of them says the connection is gone, the tail takes that link's type, SQLState and vendor code. Where none does, the tail stays the bareSQLExceptionit was, so the cut makes no failure a drop either (the concerncatalogConnectTimedOut()'s javadoc spells out about manufacturing class 08).JDBCStorage.saysTheConnectionIsGone()becomes package-private so that the rebuild asks the very question the classifier asks, instead of a second copy of it.Kept as they were: the 32-link budget of the rebuild and of
holdsCredentials(), and the "left out" tail message (testTheTailOfALongChainIsNamedRatherThanDroppedis untouched and green). A memoised rebuild with no budget was considered and not taken: it recurses as deep as the chain, and the budget is also what bounds that.Tests
CachedConnectionTestCase:testALinkThatSaysTheConnectionIsGoneByItsTypeKeepsItThroughRedaction- each of the three connection types, no SQLState, behind a link naming the password.testALinkThatSaysTheConnectionIsGonePastTheBudgetOfARebuildStillSaysSo- 08S01, and a statelessSQLRecoverableException, as the 41st link; url without a password.testALongChainThatSaysNothingOfTheConnectionIsNotMadeToSayItByTheRebuild- guard: 40 plain links stayfalse.CatalogConnectionTestCase(end to end throughnewCatalogConnection()):testARefusalThatSaysTheConnectionIsGoneDeepInItsChainStillSaysSo- direct throw.testATimedOutRefusalThatSaysTheConnectionIsGoneDeepInItsChainStillSaysSo- 53300 retried to the deadline, 08S01 deep in its next chain.Before the fix: 4 of the 5 red (the guard green, as a guard should be). After:
CachedConnectionTestCase120/120,CatalogConnectionTestCase20/20,JDBCStorageRetryTest113/113,StampConnectionTestCase6/6,Skipped: 0.Mutants, each red on its own case:
SQLExceptionagain…ByItsTypeKeepsItThroughRedaction…PastTheBudgetOfARebuildStillSaysSo+ bothCatalogConnectionTestCasecases…IsNotMadeToSayItByTheRebuild…PastTheBudgetOfARebuildStillSaysSo